Skip to content

snapshot: fix listSnapshots when storage pool is removed #12433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
yadvr wants to merge 1 commit into
base: 4.22
Choose a base branch
from
Open

snapshot: fix listSnapshots when storage pool is removed #12433

yadvr wants to merge 1 commit into from
+1 −1

Conversation

@yadvr
Copy link
Member

@yadvr yadvr commented Jan 15, 2026

This fixes the case when the storage pool is removed as well the KVM host and the subsequent volumes on the host. When that happened, listing snapshots (for recovery purposes) cause NPE as the pool_id was null, but last_pool_id for the related destroyed volume wasn't null. This adds a fallback logic.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@yadvr yadvr added this to the 4.22.1 milestone Jan 15, 2026
@yadvr
Copy link
Member Author

yadvr commented Jan 15, 2026

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jan 15, 2026

@yadvr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link

codecov bot commented Jan 15, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 17.60%. Comparing base (b1f870a) to head (9dd8a4c).

Files with missing lines Patch % Lines
...torage/snapshot/StorageSystemSnapshotStrategy.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##               4.22   #12433   +/-   ##
=========================================
  Coverage     17.59%   17.60%           
  Complexity    15611    15611           
=========================================
  Files          5910     5910           
  Lines        529890   529890           
  Branches      64737    64738    +1     
=========================================
+ Hits          93259    93262    +3     
+ Misses       426135   426132    -3     
  Partials      10496    10496
Flag Coverage Δ
uitests 3.60% <ø> (ø)
unittests 18.66% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DaanHoogland
DaanHoogland approved these changes Jan 15, 2026
View reviewed changes
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, (no doubt even more fallback can be added..?? i.e. volumeStoragePoolId == null than throw a suitable CloudRuntimeException)

sureshanaparti
sureshanaparti approved these changes Jan 15, 2026
View reviewed changes
Copy link
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

@blueorangutan
Copy link

blueorangutan commented Jan 15, 2026

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16382

@yadvr
Copy link
Member Author

yadvr commented Jan 15, 2026

@blueorangutan test

@yadvr
Copy link
Member Author

yadvr commented Jan 21, 2026

@blueorangutan help

@blueorangutan
Copy link

blueorangutan commented Jan 21, 2026

@yadvr [SL] I understand these words: "help", "hello", "thanks", "package", "test"
Test command usage: test [mgmt os] [hypervisor] [keepEnv] [qemuEv] [basicZone|securityGroups]
Mgmt OS options: ['suse15', 'alma10', 'ol10', 'rocky10', 'alma9', 'centos7', 'centos6', 'rocky9', 'alma8', 'ubuntu18', 'ol9', 'ol8', 'ubuntu22', 'debian12', 'ubuntu20', 'rocky8', 'ubuntu24']
Hypervisor options: ['kvm-centos6', 'kvm-centos7', 'kvm-rocky8', 'kvm-rocky9', 'kvm-rocky10', 'kvm-ol8', 'kvm-ol9', 'kvm-ol10', 'kvm-alma8', 'kvm-alma9', 'kvm-alma10', 'kvm-ubuntu18', 'kvm-ubuntu20', 'kvm-ubuntu22', 'kvm-ubuntu24', 'kvm-debian12', 'kvm-suse15', 'vmware-55u3', 'vmware-60u2', 'vmware-65u2', 'vmware-67u3', 'vmware-70u1', 'vmware-70u2', 'vmware-70u3', 'vmware-80', 'vmware-80u1', 'vmware-80u2', 'vmware-80u3', 'vmware-80u3e', 'xenserver-65sp1', 'xenserver-71', 'xenserver-74', 'xenserver-84', 'xcpng74', 'xcpng76', 'xcpng80', 'xcpng81', 'xcpng82', 'xcpng83']
Note: when keepEnv is passed, you need to specify mgmt server os and hypervisor or use the matrix command.
when qemuEv is passed, it will deploy KVM hyperviosr hosts with qemu-kvm-ev, else it will default to stock qemu.
When basicZone and/or securityGroups are passed it will create a zone of the last type specified (default is Advanced)
Package command usage: package [all(default value),kvm,xen,vmware,hyperv,ovm] - a comma separated list can be passed with package command to bundle the required hypervisor's systemVM templates. Not passing any argument will bundle all - kvm,xen and vmware templates.

Blessed contributors for kicking Trillian test jobs: ['rohityadavcloud', 'shwstppr', 'Damans227', 'vishesh92', 'Pearl1594', 'harikrishna-patnala', 'nvazquez', 'DaanHoogland', 'weizhouapache', 'borisstoyanov', 'vladimirpetrov', 'kiranchavala', 'andrijapanicsb', 'NuxRo', 'rajujith', 'alexandremattioli', 'sureshanaparti', 'abh1sar', 'sudo87', 'RosiKyu']

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jan 21, 2026

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Jan 21, 2026

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

blueorangutan commented Jan 21, 2026

[SF] Trillian Build Failed (tid-15229)

@blueorangutan
Copy link

blueorangutan commented Jan 22, 2026

[SF] Trillian test result (tid-15230)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 51217 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12433-t15230-kvm-ol8.zip
Smoke tests completed. 146 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestClusterDRS>:setup Error 0.00 test_cluster_drs.py
test_03_deploy_and_scale_kubernetes_cluster Failure 26.80 test_kubernetes_clusters.py
test_list_system_vms_metrics_history Failure 0.21 test_metrics_api.py

@yadvr @DaanHoogland
snapshot: fix listSnapshots for volume which got delete and whose sto…
9dd8a4c 
…rage pool got deleted

This fixes the case when the storage pool is removed as well the KVM
host and the subsequent volumes on the host. When that happened, listing
snapshots (for recovery purposes) cause NPE as the pool_id was null, but
last_pool_id for the related destroyed volume wasn't null. This adds a
fallback logic.

Signed-off-by: Rohit Yadav <rohit@yadav.cloud>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@sureshanaparti sureshanaparti sureshanaparti approved these changes

@harikrishna-patnala harikrishna-patnala Awaiting requested review from harikrishna-patnala

@nvazquez nvazquez Awaiting requested review from nvazquez

@weizhouapache weizhouapache Awaiting requested review from weizhouapache

Assignees

@borisstoyanov borisstoyanov

Labels

component:storage status:needs-testing

Projects

Apache CloudStack 4.22.1
Status: In Progress

Milestone

4.22.1

Development

Successfully merging this pull request may close these issues.

5 participants

@yadvr @blueorangutan @DaanHoogland @sureshanaparti @borisstoyanov